[codex] Fix time interval arithmetic wrapping#22280
Open
Sean-Kenneth-Doherty wants to merge 2 commits into
Open
[codex] Fix time interval arithmetic wrapping#22280Sean-Kenneth-Doherty wants to merge 2 commits into
Sean-Kenneth-Doherty wants to merge 2 commits into
Conversation
Author
|
Validation completed before moving this out of draft:
Note: the first clippy attempt failed because native C compilation scratch files hit a local |
Author
|
Follow-up pushed after final local review: the arithmetic now wraps in nanoseconds before reducing back to the time unit, which covers negative sub-second intervals for lower-precision time values. Added regression:
Reran validation after the follow-up:
|
Author
|
The PR body is updated with the latest follow-up and validation. Only the labeler workflow has run so far; could a committer please trigger the full CI when convenient? Latest local validation:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
time +/- intervalandinterval + timereturn a time value instead of an intervalarith_time_interval.sltwith PostgreSQL-compatible wrapping coverage, including the reported23:30 + 2 hourscase and aTime32(Second)sub-second regressionFixes #22265.
Rationale for this change
time +/- intervalshould stay in the 24-hour time-of-day domain. DataFusion currently routes these expressions throughInterval(MonthDayNano), which can produce interval values such as25 hours 30 minsinstead of wrapping to01:30:00.What changes are included in this PR?
time + interval,interval + time, andtime - intervalso the result type remains the original time type.Are these changes tested?
cargo test --profile=ci --test sqllogictests -- arith_time_interval.sltcargo fmt --all --checkgit diff --check origin/main...HEADcargo clippy --all-targets --all-features -- -D warningsAre there any user-facing changes?
Yes.
time +/- intervalandinterval + timenow return wrapped time values instead of interval values.AI-assisted contribution note
I used OpenAI Codex while developing and validating this PR. I reviewed the implementation end-to-end. The main assumption to call out is that for time-of-day arithmetic, interval month/day fields are whole-day-or-larger components and therefore have no effect after modulo-24-hour wrapping; the nanosecond field is applied at full precision before reducing to the specific Time32/Time64 unit.